-
Notifications
You must be signed in to change notification settings - Fork 210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add otelcol.processor.groupbyattrs
component
#1300
Add otelcol.processor.groupbyattrs
component
#1300
Conversation
@grafana/grafana-alloy-maintainers Can we do a code review before I review the doc? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! I left a few comments. Let me know when this is ready for a second review :)
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
internal/component/otelcol/processor/groupbyattrs/groupbyattrs.go
Outdated
Show resolved
Hide resolved
internal/component/otelcol/processor/groupbyattrs/groupbyattrs.go
Outdated
Show resolved
Hide resolved
internal/component/otelcol/processor/groupbyattrs/groupbyattrs.go
Outdated
Show resolved
Hide resolved
internal/component/otelcol/processor/groupbyattrs/groupbyattrs.go
Outdated
Show resolved
Hide resolved
You will also need to add a config converter for the component. You can follow the pattern used for the other otel components in https://github.com/grafana/alloy/tree/main/internal/converter/internal/otelcolconvert |
- Use Generally avaialble stability - Import Correct package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some doc suggestions
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
# Conflicts: # CHANGELOG.md # go.mod # go.sum
@wildum could you please take another look? |
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code looks good to me, I just have some little feedback on the doc. I found that the examples in the original doc were helpful to understand how the processor work. Should we add some examples or is it already clear enough? @ptodev I think you also wanted to give some feedback on this PR?
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
internal/component/otelcol/processor/groupbyattrs/groupbyattrs.go
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Show resolved
Hide resolved
internal/converter/internal/otelcolconvert/testdata/groupbyattrs.alloy
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
internal/component/otelcol/processor/groupbyattrs/groupbyattrs.go
Outdated
Show resolved
Hide resolved
…rs.alloy Co-authored-by: Paulin Todev <paulin.todev@gmail.com>
…nto otelcol-processor-groupbyattr # Conflicts: # docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md # internal/converter/internal/otelcolconvert/testdata/groupbyattrs.alloy
Thanks for the review team. I'm sorry the second set of changes came a bit later. I have been busy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @kehindesalaam! When @clayton-cornell approves it and when the CI is green, I'm happy for the PR to be merged!
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.processor.groupbyattrs.md
Outdated
Show resolved
Hide resolved
…oupbyattrs.md Co-authored-by: Paulin Todev <paulin.todev@gmail.com>
…oupbyattrs.md Co-authored-by: Paulin Todev <paulin.todev@gmail.com>
…oupbyattrs.md Co-authored-by: Paulin Todev <paulin.todev@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Looks good to me. 🚢 I just pushed a commit on your branch with a few minor docs fixes. By the way, you can visualise the docs in a browser by running these commands:
cd docs
make docs
I find this very useful when I want to make sure everything gets rendered appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc are ok
PR Description
Adds the
otelcol.processor.groupbyattrs
component which allows grouping of metrics matching specific attributes.Which issue(s) this PR fixes
Fixes #224
Notes to the Reviewer
PR Checklist